-
-
Notifications
You must be signed in to change notification settings - Fork 29
fix compatible issues in python 3.14 #236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ork' Python 3.14 changed the default multiprocessing start method from 'fork' to 'forkserver' on Linux, which breaks SIGCHLD signal delivery in tests. This causes pselect tests to hang indefinitely. Changes: - Add Python 3.14 detection in conftest.py - Force multiprocessing to use 'fork' method for Python 3.14+ - Add defensive check for __test__ attribute before deletion Fixes test timeouts on Python 3.14.0rc2. All tests now pass in ~8 seconds (previously timed out at 300s+).
…ndlers Python 3.14 changed recursion checking to use actual C stack pointers instead of counters. The old implementation called Python code from within signal handlers before siglongjmp(), causing _Py_CheckRecursiveCall() to see inconsistent stack pointers and report 'Unrecoverable stack overflow'. Solution: Never call Python code from within signal handlers. Changes: - Added signal_to_raise field to cysigs_t to store deferred signal - Modified signal handlers to only set flag and longjmp (no Python calls) - Added _sig_raise_deferred() to raise exceptions after longjmp - Updated _sig_on_postjmp() to call _sig_raise_deferred() in safe context This maintains async-signal-safety and ensures Python's stack tracking remains consistent. All 65 tests pass on Python 3.14.0rc3. Also updates: - pyproject.toml: requires-python = '>=3.9,<3.15' (was <3.14) - CI/CD: Add Python 3.14 to test matrix
@tobiasdiez I have tried to fix the signal handle in python 3.14 https://github.com/cxzhong/cysignals/actions/runs/18195804743 |
- Remove forced fork() from conftest.py to allow Python 3.14's default forkserver multiprocessing start method - Add PSelecter.wait_for_process() method that monitors process.sentinel instead of SIGCHLD, working with any start method (fork/spawn/forkserver) - Update doctests to demonstrate both SIGCHLD-based (fork-only) and sentinel-based (universal) process monitoring approaches - Note: wait_for_process() is POSIX-only; Windows should use multiprocessing.connection.wait() instead This allows cysignals to work properly with Python 3.14's new forkserver default while maintaining backward compatibility.
…refcnt - Replace direct ob_refcnt access with _Py_REFCNT() macro from cpython.ref - Add NULL checks before calling _Py_REFCNT() to prevent null pointer dereference - This fixes compilation errors in Python 3.14t (free-threaded/nogil) build where ob_refcnt is not directly accessible in PyObject structure
262520b
to
546217f
Compare
…_REFCNT - Remove _Py_REFCNT from cpython.ref import (not available in Cython) - Declare Py_REFCNT as external C function from Python.h - Replace _Py_REFCNT() calls with Py_REFCNT() (2 occurrences) - This properly supports Python 3.14t free-threaded build where ob_refcnt is not directly accessible in PyObject structure
fix #232 and timeout problem in Linux. and I have finally fixed the problems building in python3.14t. The timeout problem is caused by changing of default start method of python 3.14 multiprocess from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot!
Mostly looks good to me, based on my superficial understanding of the code. @dimpase @ptrrsn @user202729 could you please have a look at this PR as well?
@tobiasdiez can you let the workflows run? Thank you. |
Remove unused import of sys module.
Replaced usage of Py_REFCNT with _Py_REFCNT for compatibility with Python 3.14t free-threaded builds.
Now all tests pass. https://github.com/cxzhong/cysignals/actions/runs/18242832091 |
instead of adding The less states there are, the less likely it is to get inconsistent. Is there any situation where What does |
also, disclose your extent of usage of AI based tools. (most human don't write the pull request description the way you did.) |
I have rewrited the PR description. |
Eliminate redundant signal state by using only sigsetjmp/siglongjmp return values to propagate signal numbers. This simplifies the signal handling architecture and prevents potential desynchronization bugs. Changes: - Remove signal_to_raise field from cysigs_t structure - Update signal handlers to not store signal number in global variable - Remove _sig_raise_deferred() function - Modify _sig_on_postjmp() to directly use jump return value - Update Cython declarations accordingly Benefits: - Eliminates redundant state (16 lines removed) - Prevents state desynchronization issues - Signal number propagates naturally via jump mechanism - All 66 tests pass successfully The refactoring maintains full backward compatibility with no functional changes to the API or behavior.
any idea if this affect anything on WIN32? Given that the edit: looks like tests pass, so it probably can't be too bad. |
Maybe we need to test the compatibility in sagemath. To see everything is OK. Thank you very much. |
for SageMath on Python 3.14 we need to fix atexit.pyx first, before we move on to other things |
|
uv updates are fine. |
|
Removed the wait_for_process method that monitored process exit using a sentinel file descriptor.
I have finished tests on nogil python 3.14t in Linux, it can pass all tests if we run |
Updated Python version in CI workflow from '3.14.0-rc.3' to '3.14'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Align cysignals with Python 3.14 and Cython 3.1, avoiding Python calls in signal handlers, adapting to multiprocessing start-method changes, and updating refcount access for CPython changes.
- Defer exception raising from signal handlers; use siglongjmp to return and raise in a safe context.
- Replace direct ob_refcnt with _Py_REFCNT and add Cython free-threading compatibility directives; bump Python/Cython minimums.
- Update docs/tests to use multiprocessing.get_context and process sentinel; refresh CI runners and Python versions.
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
src/cysignals/tests.pyx | Add Cython free-threading compatibility directive. |
src/cysignals/signals.pyx | Use _Py_REFCNT and import; add extern for _do_raise_exception. |
src/cysignals/signals.pxd | Declare _do_raise_exception (currently under a nogil block). |
src/cysignals/pysignals.pyx | Add Cython free-threading compatibility directive. |
src/cysignals/pselect.pyx | Doctest updates to use multiprocessing.get_context and sentinels. |
src/cysignals/macros.h | Raise exception in safe context via _do_raise_exception in _sig_on_postjmp. |
src/cysignals/implementation.c | Avoid Python calls in signal handlers; rename do_raise_exception to _do_raise_exception. |
src/cysignals/alarm.pyx | Add Cython free-threading compatibility directive. |
src/conftest.py | Safely delete test only if present. |
pyproject.toml | Bump Cython/Python requirements; update classifiers. |
README.rst | Update requirements to Python ≥3.11 and Cython ≥3.1. |
.github/workflows/dist.yml | Update macOS runner to 14. |
.github/workflows/ci.yml | Add Python 3.14 to matrix; update macOS runner. |
Comments suppressed due to low confidence (3)
src/cysignals/implementation.c:1
- On Windows, the signal handler no longer raises an exception or returns via a trampoline; the previous do_raise_exception(sig) call was removed and there's no alternative in the _WIN32 path. Add an #else branch that calls _do_raise_exception(sig) on Windows to avoid losing interrupts.
/*
src/cysignals/implementation.c:1
- Same issue for non-SIGINT signals: on Windows, the handler now does neither a longjmp nor raises an exception. Add an _WIN32 branch that calls _do_raise_exception(sig) to preserve behavior on Windows.
/*
src/cysignals/signals.pxd:1
- _do_raise_exception raises Python exceptions and must run with the GIL, but it's declared inside a cdef nogil block, permitting calls without the GIL. Move this declaration out of the nogil block (declare it under a normal cdef:) or ensure _do_raise_exception acquires/releases the GIL internally in all code paths.
# cython: preliminary_late_includes_cy28=True
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
static void cysigs_signal_handler(int sig); | ||
|
||
static void do_raise_exception(int sig); | ||
static void _do_raise_exception(int sig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_do_raise_exception is declared static here, but it is invoked from macros.h's inline _sig_on_postjmp, which is compiled into other translation units. With static linkage, other TUs cannot resolve this symbol, causing link errors. Remove static (make it have external linkage) and provide a prototype in a shared header (e.g., macros.h) so all TUs see the declaration.
static void _do_raise_exception(int sig); | |
void _do_raise_exception(int sig); |
Copilot uses AI. Check for mistakes.
/* A signal occurred and we jumped back via longjmp. | ||
* jmpret contains the signal number that was passed to siglongjmp. | ||
* Now we're back in a safe context (not in signal handler), | ||
* so it's safe to call Python code to raise the exception. */ | ||
_do_raise_exception(jmpret); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This inline function calls _do_raise_exception but there's no C declaration visible in this header. Without an extern prototype, this will be an implicit declaration (or an error under modern compilers). Add an extern prototype for _do_raise_exception(int) in a header included by all consumers (e.g., declare it at the top of macros.h) and ensure the function has external linkage.
Copilot uses AI. Check for mistakes.
* Return 0 if there was an exception, 1 otherwise. | ||
* | ||
* This function propagates the signal number naturally via jmpret | ||
* (the return value from sigsetjmp/cylongjmp), which is always nonzero |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct 'cylongjmp' to 'siglongjmp'.
* (the return value from sigsetjmp/cylongjmp), which is always nonzero | |
* (the return value from sigsetjmp/siglongjmp), which is always nonzero |
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
This PR fixes the compatible problem with python 3.14.
signal_to_raise
record the signal and call python exception in the safety context. (fix it with more straight way)multiprocess
in python 3.14. since in python 3.14, the default start method ofmultiprocess
has changed fromfork
toforkserver
. So, in the multiprocess tests inpselect.pyx
, we can not get the signal ofSIGCHLD
. It will cause the timeout problem in linux. But in macos, theforkserver
has different behavior with the behavior in linux. Maybe thewait_for_process
is not useful. I just want to monitor the process sentinel file descriptor instead ofSIGCHLD
.ob_refcnt
access with_Py_REFCNT()
macro fromcpython.ref